-
-
Notifications
You must be signed in to change notification settings - Fork 363
perf(Message): improve performance #7094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideConsolidated Message component rendering logic, optimized JS interop and event handling, refactored per-item clear/dismiss operations, introduced ForceDelay override with dynamic default delay support, and updated unit tests and sample demos accordingly. Sequence diagram for per-item clear and dismiss operationssequenceDiagram
participant JS as "Message.razor.js"
participant Message as "Message (C#)"
participant User as actor "User"
User->>JS: Clicks dismiss button
JS->>Message: Dismiss(alertId)
Message->>Message: Remove MessageOption by id
Message->>Message: StateHasChanged()
Message-->>JS: (async completion)
User->>JS: Message auto-hides
JS->>Message: Clear(msgId)
Message->>Message: Remove MessageOption by id
Message->>Message: StateHasChanged()
Class diagram for updated Message component logicclassDiagram
class Message {
-List<MessageOption> _messages
-string _msgId
+void Clear(string id)
+ValueTask Dismiss(string id)
+void SetPlacement(Placement placement)
+List<MessageOption> GetMessages()
+Task Show(MessageOption option)
}
class MessageOption {
+string Content
+string Icon
+bool ShowDismiss
+bool IsAutoHide
+int Delay
+RenderFragment ChildContent
+Func<Task> OnDismiss
+bool ShowShadow
+MessageShowMode ShowMode
}
Message "1" *-- "*" MessageOption : manages
class Placement {
<<enum>>
Top
Bottom
}
MessageOption --> Placement : uses
Class diagram for dynamic default delay configurationclassDiagram
class BootstrapBlazorOptions {
+int MessageDelay
}
class IOptionsMonitor {
+BootstrapBlazorOptions CurrentValue
}
class Message {
+int DefaultDelay
}
Message --> IOptionsMonitor : uses for delay
IOptionsMonitor --> BootstrapBlazorOptions : provides
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/Message/Message.razor.js:48-49` </location>
<code_context>
el.classList.add('show');
const close = () => {
- EventHandler.off(el, 'click')
- el.classList.remove('show');
- const hideHandler = setTimeout(function () {
</code_context>
<issue_to_address>
**suggestion:** The close function is now unused and can be removed for clarity.
Removing unused code helps maintain clarity and reduces potential confusion.
</issue_to_address>
### Comment 2
<location> `test/UnitTest/Components/MessageTest.cs:156-161` </location>
<code_context>
+ ForceDelay = true,
+ Delay = 2000
+ };
+ await cut.InvokeAsync(() => service.Show(option, cut.Instance));
+ Assert.Contains("data-bb-delay=\"2000\"", cut.Markup);
+
+ var alert = cut.Find(".alert");
</code_context>
<issue_to_address>
**suggestion (testing):** Add assertions to verify that messages are actually removed from the DOM after Clear is called.
Consider adding an assertion to confirm that the alert element is removed from the DOM after Clear is called, to fully validate the Clear functionality.
```suggestion
await cut.InvokeAsync(() => cut.Instance.Clear(alert.Id));
// Assert that the alert element is removed from the DOM after Clear is called
Assert.Empty(cut.FindAll(".alert"));
option.ForceDelay = false;
await cut.InvokeAsync(() => service.Show(option, cut.Instance));
Assert.Contains("data-bb-delay=\"4000\"", cut.Markup);
await cut.InvokeAsync(() => cut.Instance.Clear(alert.Id));
// Assert that the alert element is removed from the DOM after Clear is called
Assert.Empty(cut.FindAll(".alert"));
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7094 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 745 745
Lines 32556 32542 -14
Branches 4512 4509 -3
=========================================
- Hits 32556 32542 -14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the Message component to improve performance by simplifying the component lifecycle and reducing unnecessary re-renders. The changes streamline message management by removing the callback mechanism and making the Clear method accept a specific message ID instead of clearing all messages.
Key changes include:
- Refactored JavaScript interop to remove the callback parameter and call Clear/Dismiss methods directly with message IDs
- Modified the Razor template to eliminate duplicate code by using a single loop with conditional reversal logic
- Updated the Clear method to remove individual messages by ID rather than clearing all messages
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BootstrapBlazor/Components/Message/Message.razor.js | Removed callback parameter from init, refactored close logic to call Clear with message ID, updated event handlers |
| src/BootstrapBlazor/Components/Message/Message.razor.cs | Changed Clear to accept message ID parameter, updated Dismiss signature to ValueTask, added GetMessages helper method |
| src/BootstrapBlazor/Components/Message/Message.razor | Simplified template by removing duplicate code and using GetMessages() for conditional ordering |
| test/UnitTest/Components/MessageTest.cs | Updated test calls to wrap Clear in InvokeAsync, added ForceDelay_Ok test for delay behavior |
| src/BootstrapBlazor.Server/Components/Samples/Messages.razor.cs | Updated demo to use message counter and consolidated message instance references |
| src/BootstrapBlazor.Server/Components/Samples/Messages.razor | Enhanced demo UI with placement selector |
| exclusion.dic | Added spelling exclusions for 'alertdialog' and 'blazorbootstrap' |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@sourcery-ai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- The sample’s
_itemslist is initialized with square brackets instead of a valid C# collection initializer (use{}), which will fail to compile. - The JS interop setup doesn’t clean up event handlers or Data entries on component disposal; consider implementing
DisposeAsyncto remove handlers and avoid memory leaks. - The fallback to a 1000ms delay when
MessageDelayis set to zero can be surprising—consider making this default behavior more explicit in code or via a named constant.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The sample’s `_items` list is initialized with square brackets instead of a valid C# collection initializer (use `{}`), which will fail to compile.
- The JS interop setup doesn’t clean up event handlers or Data entries on component disposal; consider implementing `DisposeAsync` to remove handlers and avoid memory leaks.
- The fallback to a 1000ms delay when `MessageDelay` is set to zero can be surprising—consider making this default behavior more explicit in code or via a named constant.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/Message/Message.razor.js:55-64` </location>
<code_context>
};
- EventHandler.on(el, 'click', '.btn-close', e => {
+ EventHandler.on(el, 'click', '.btn-close', async e => {
e.preventDefault();
e.stopPropagation();
const alert = e.delegateTarget.closest('.alert');
if (alert) {
+ EventHandler.off(el, 'click')
+ alert.classList.add("d-none");
+
const alertId = alert.getAttribute('id');
- msg.invoke.invokeMethodAsync('Dismiss', alertId);
+ msg.items = msg.items.filter(i => i.el.id !== alertId);
+ await msg.invoke.invokeMethodAsync('Dismiss', alertId);
}
- close();
</code_context>
<issue_to_address>
**issue (bug_risk):** Mixing async/await with non-async event handler registration may cause unhandled promise rejections.
Wrap the async handler logic in a try/catch block to prevent silent UI failures from unhandled promise rejections.
</issue_to_address>
### Comment 2
<location> `test/UnitTest/Components/MessageTest.cs:170-150` </location>
<code_context>
+ Assert.Contains("data-bb-delay=\"1000\"", cut.Markup);
+ await cut.InvokeAsync(() => cut.Instance.Clear(alert.Id));
+
+ options.CurrentValue.MessageDelay = 0;
+ await cut.InvokeAsync(() => service.Show(option, cut.Instance));
+ Assert.Contains("data-bb-delay=\"1000\"", cut.Markup);
+ await cut.InvokeAsync(() => cut.Instance.Clear(alert.Id));
</code_context>
<issue_to_address>
**suggestion (testing):** Suggestion: Add a test for negative MessageDelay values.
Add a test with a negative MessageDelay to verify the component's behavior and ensure it handles invalid input correctly.
Suggested implementation:
```csharp
options.CurrentValue.MessageDelay = 1000;
await cut.InvokeAsync(() => service.Show(option, cut.Instance));
Assert.Contains("data-bb-delay=\"1000\"", cut.Markup);
await cut.InvokeAsync(() => cut.Instance.Clear(alert.Id));
// Negative MessageDelay test
options.CurrentValue.MessageDelay = -500;
await cut.InvokeAsync(() => service.Show(option, cut.Instance));
// Assert that negative delay is handled (e.g., defaults to 0 or a minimum value)
Assert.DoesNotContain("data-bb-delay=\"-500\"", cut.Markup);
Assert.Contains("data-bb-delay=\"0\"", cut.Markup); // Adjust if the component defaults to another value
await cut.InvokeAsync(() => cut.Instance.Clear(alert.Id));
```
If the component does not default negative values to 0, adjust the assertion to match the actual default value. You may also want to check the component's implementation to ensure negative values are handled gracefully.
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Components/Message/Message.razor.cs:88` </location>
<code_context>
}
}
+ private List<MessageOption> GetMessages()
+ {
+ var messages = new List<MessageOption>(_messages);
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the message list reversal and removal logic with a property and helper method to streamline rendering and message management.
Here’s one way to collapse the duplication and simplify the “reverse‐if‐bottom” logic without losing any behavior:
```csharp
// replace GetMessages()
private IEnumerable<MessageOption> MessagesForRender =>
Placement == Placement.Bottom
? _messages.AsEnumerable().Reverse()
: _messages;
// helper to remove + return the removed item
private MessageOption? RemoveMessageById(string id)
{
var msg = _messages.FirstOrDefault(x => GetItemId(x) == id);
if (msg != null)
_messages.Remove(msg);
return msg;
}
[JSInvokable]
public Task Clear(string id)
{
RemoveMessageById(id);
StateHasChanged();
return Task.CompletedTask;
}
[JSInvokable]
public async ValueTask Dismiss(string id)
{
var msg = RemoveMessageById(id);
if (msg?.OnDismiss != null)
await msg.OnDismiss();
StateHasChanged();
}
```
And in your `.razor` just enumerate `MessagesForRender` instead of calling `GetMessages()`. This:
- Removes the extra copy/reverse helper.
- Unifies the “find & remove” logic into one small method.
- Still calls all callbacks and updates state exactly once.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Link issues
fixes #7093
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Streamline the Message component's rendering and interop layers, add per-message force delay support, and update tests and samples accordingly.
New Features:
Enhancements:
Tests:
Chores: